Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement #1495, Method call chaining #3263

Merged
merged 2 commits into from
Nov 28, 2013
Merged

Conversation

xixixao
Copy link
Contributor

@xixixao xixixao commented Nov 27, 2013

Implements #1495 and parts of its sibling issues.

@jashkenas was against this 3 years ago saying:

This has come up before, and I'm still of the opinion that it's an abuse of indentation. Leading dot accessors should attach to the final value of the previous line, and not cause the previous line to be implicitly wrapped in parentheses.

This sounds reasonable, but in practice I was unable to find any occurrences of the pattern

a b
.c() #a(b.c())

vs hundreds of cases of

a.b(c)
 .d(e)

This has also been highly requested and discussed syntax. There have been many proposals, a lot of them dealing with indentation. I think that we can agree that the change should be as simple and useful as possible.

There are several examples already where our code looks similar to the proposal:

a b,
  c
.something() # a(b, c).something()

a
  b: c
.something()  # a(b: c).something()

a ->
  b c
.something()  # a(function () {return b(c);}).something()

This PR brings normal argument list on par with these, and also solves cases of nested calls and inline functions. This is now valid:

result = range 1, 3
  .concat range 4, 6
  .map (x) -> x * x
  .filter (x) -> x % 2 is 0

console.log result # [4, 16, 36]

Yet the syntax is indentation agnostic. The rule is simply that a dot accessor on a new line closes all implicit calls, or that it "attaches to the outermost preceding value".

oldschool P.S.:

$ 'body'
.click (e) ->
  $ '.box'
  .fadeIn 'fast'
  .addClass '.active'
  .css 'marginRight', '10px'

@michaelficarra
Copy link
Collaborator

Wow, I didn't think we'd ever see a PR for this. Nice job, I'll review soon.

@jashkenas
Copy link
Owner

The rule is simply that a dot accessor on a new line closes all implicit calls

All of em?

What happens in cases like this?

function arg, ->
  jQuery selector
    .fadeIn 'slow'

@vendethiel
Copy link
Collaborator

@jashkenas I had the same question, but then I re-read the last example :

$ 'body'
.click (e) ->
$ '.box'
.fadeIn 'fast'
.addClass '.active'
.css 'marginRight', '10px'

@xixixao
Copy link
Contributor Author

xixixao commented Nov 27, 2013

@jashkenas Only implicit calls, not function bodies or object bodies. So in your case the "open" function body prevents closing of the first function call. I agree explaining it in words is tougher than showing by examples / the algorithm itself, but it should be easy to grasp.

@nfour
Copy link

nfour commented Nov 28, 2013

+1. The current trends with promises makes using coffeescript somewhat pointless when chaining as we need to wrap with parenthesis or assign temp variables.

Chaining like this would be highly pleasing, seems like one of the few remaining JS syntax constructs CS is failing to improve on. I could imagine it would be more fitting for redux though considering backwards compat.

@xixixao
Copy link
Contributor Author

xixixao commented Nov 28, 2013

I could imagine it would be more fitting for redux though considering backwards compat.

If we were using semver we would be on version 7.0.1. There are loads of backwards incompatible changes (the changes to string literals I worked on for example) on master. I also hope that #2887 will get merged in before 1.7.0 (more of a big change rather than backwards incompatible). The main point is I couldn't find the code that changes behavior in production, any examples? Redux is not tangible for me right now.

I totally agree about promises.

@nfour
Copy link

nfour commented Nov 28, 2013

@xixixao I simply imagine backwards compatibility would be the only practical obstacle for this feature. I've followed this chaining syntax debate for a year or so and the only other reason that comes to mind is that the big CS contributors just don't like it, or care. I'd imagine their opinions may have changed, though. Heres to hoping :D

@jashkenas
Copy link
Owner

Here we go...

jashkenas added a commit that referenced this pull request Nov 28, 2013
Implement #1495,  Method call chaining
@jashkenas jashkenas merged commit 563f14b into jashkenas:master Nov 28, 2013
@raganwald
Copy link
Contributor

👍

@charltoons
Copy link

👍 👍

endAllImplicitCalls = ->
while inImplicitCall()
endImplicitCall()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should probably be a return here, to avoid storing/returning the value of the comprehension.

vendethiel added a commit that referenced this pull request Dec 5, 2013
Fixup #3263: Prevent loop collection in endAllImplicitCalls
@00dani
Copy link

00dani commented Dec 6, 2013

This looks great! When can we expect to see a release containing this change? It'll definitely work wonderfully for promises, as well as a bunch of other chainy interfaces (jQuery's for example).

@shesek
Copy link

shesek commented Dec 6, 2013

I think I have some cases in my code-base that relied on the old behavior. Can someone perhaps make some tool to find those cases, or possibly even issue a warning with the file/line and a notice about what changed when CoffeeScript encounter those cases in the next 2-3 releases, until everyone are aware of it?

I can see this causing some havoc... we at least need an easy way to let people find those cases before they upgrade.

@xixixao
Copy link
Contributor Author

xixixao commented Dec 6, 2013

@shesek I'd simply compile your codebase with both versions (you can do this now with last release and master) and diff the outputted JS. I'd be interested to see the use cases.

@simoami
Copy link

simoami commented Jan 8, 2014

glad to see method chaining finally happening. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants